-
Notifications
You must be signed in to change notification settings - Fork 369
Enable Miri tests in CI with non-blocking execution #697
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com>
Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com>
Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com>
Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com>
|
|
||
| #[test] | ||
| fn test_block_transpose_16() { | ||
| #[cfg(not(miri))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these bounds be set up with something like
let row_range = if cfg!(miri) {
127..128
} else {
0..128
};and such to avoid the repitition?
| } | ||
|
|
||
| #[test] | ||
| fn test_distances_in_place() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably skip this test entirely when running under miri. There is a dedicated Miri test just after this that validates the indexing.
| } | ||
| } | ||
|
|
||
| #[cfg(miri)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one we cannot short circuit with Miri. The distances_in_place algorithm needs to be tested for a range of sizes to validate memory accesses.
| // | ||
| // Subsampling results in poor preservation of inner products, so we skip it | ||
| // altogether. | ||
| #[cfg(not(miri))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be tempted to say that we can skip all of test_double_hadamard when running under Miri. It doesn't execute any unsafe code on its own.
|
|
||
| // Miri is extremely slow, so we skip the larger tests there. | ||
| #[cfg(not(miri))] | ||
| let dim_combos = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with test_double_hadamard, I'm tempted to say we can skip test_padding_hadamard altogether under Miri.
| #[cfg(miri)] | ||
| if dim != MAX_DIM { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can almost skip the MinMax entirely under Miri because it does not run unsafe outside of decompression. Since the indexing used in the decompression is checked in debug builds - it might be fine to just skip.
| #[cfg(miri)] | ||
| if len != MAX_DIM - 1 { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests in slice all need to run under Miri unfortunately.
| #[cfg(miri)] | ||
| if d != $dim - 1 { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably skip the min-max quantizer tests under Miri.
| } | ||
|
|
||
| #[cfg(miri)] | ||
| for total in 63..64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change the upper bound to like 48 when under Miri but still do the sweep? That seems reasonable quick.
| for num_centers in 47..48 { | ||
| test_process_into_impl(num_chunks, num_centers, 2, &mut rng); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file doesn't contain unsafe on its own, so it's probably fine to either skip miri on these test, or do what you did here on a reduced set. If the latter, we should run at least a few more intermediate values than just the last since corner-case handling needs to be covered. I'd say pick a few odd-ball sizes in between 0 and 48, for example.
Enables the previously commented-out Miri job to detect undefined behavior and memory safety issues in
diskann-quantization.Changes
continue-on-error: trueprevents Miri failures from blocking the pipelinetest-workspacejobs (depends onbasics), minimizing critical path impactcargo +nightly miri nextest run --package diskann-quantizationCI Pipeline Impact
Expected total CI time: ~23 minutes (well under 60-minute requirement). Miri adds zero time to critical path unless it becomes the longest-running parallel job.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.